docs: say a Cloud environment is protected before asking for a request - #3616
Conversation
The Deploy project page asks the reader to check the preview before
deploying, then to verify the deployment with `curl -sSf <environment-url>`.
Neither step works as written on a default Veryfront Cloud project.
Cloud creates `preview`, `staging`, and `production` as protected. A
protected environment serves only a browser signed in to Veryfront as a
project member; every other request gets a 302 to the sign-in page, on
every path including API routes, and `VERYFRONT_API_TOKEN` does not change
that because it authenticates the CLI against the Cloud API, not
deployment traffic. Against published 0.1.1229:
curl -s -o /dev/null -w '%{http_code} %{redirect_url}\n' \
https://support-agent.preview.veryfront.com/
302 https://veryfront.com/sign-in?from=%2F
The page never said so, which made the verification step worse than
useless: `curl` does not treat a 302 as a failure, so the page's own
`curl -sSf <environment-url>` exits 0 with an empty body whether or not
the deployment works. A reader following the page sees a green exit code
and no output, and concludes the deploy is fine.
Add an Environment access section between Push and Deploy — where the
reader is first told to check the preview — naming the default, the
redirect, the token's irrelevance to it, and the Studio switch that makes
an environment public. Rewrite the verification step to print the status
line, so a sign-in redirect is visible instead of passing silently, and
say outright why the bare `curl -sSf` form must not be used.
The page's other verification claim was already corrected in #3576: this
page and the sibling guide state that `veryfront open` opens the Cloud
dashboard, not the deployed site, which `veryfront open --json` on 0.1.1229
confirms (`https://veryfront.com/projects/<slug>`). That correction has not
reached the live site yet, so this change ships alongside it.
The regression test pins the three facts a reader needs — protected by
default, the sign-in redirect, and the Studio switch — plus the status-line
form of the check, and rejects a bare `curl -sSf <environment-url>` fence.
|
Warning Review limit reached
Next review available in: 37 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe deployment guide now documents protected environment access and route-specific verification. Contract tests require authentication behavior, host-dependent redirects, explicit route probing, status reporting, and API-only deployment handling. ChangesDeploy verification documentation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
Sync path confirmed, since "merged doc fixes that never reached the live site" is the recurring failure mode this round.
Worth checking after merge, in order:
One caveat found while reproducing: for this file the live page is currently ahead of |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e68f671bbb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
`lint:cwd-relative-test-reads` failed on the new case: it raised `tests/docs/guide-content.test.ts` from 29 to 30 cwd-relative reads inside test callbacks, and that ratchet may only shrink. The surrounding cases in this file are the grandfathered 29. Rather than join them, resolve the new read from `import.meta.url` — the fix the audit header prescribes, and the one #3592 applied. Test files are separate isolates sharing one process under `--parallel` and `src/testing/cwd.ts` chdirs that process, so a cwd-relative read is correct only until an unrelated file lands beside it in the same shard. Baseline is untouched: 0 at module scope, 95 in callbacks across 21 files, exactly as before.
Three review findings, all confirmed against source before applying. The claim that "every other request gets a 302" was wrong for one case. `checkProtectedProxyAccess` in `src/proxy/proxy-access-control.ts` returns 302 only while the caller is unauthenticated or its token yields no user id; a caller signed in as a user who is not a project member gets 403 instead. That is the more confusing failure of the two, because the browser is signed in and the reader would suspect the URL rather than the account, so the page now names it and says what it means. The expected `302 …` line sat inside the `bash` fence with the command. Pasting the rendered block ran `302` as a third command. It moves to its own `text` fence, per the repo rule that code examples must be safe to paste. The regression test's evidence comment recorded a real deployment hostname. Internal hostnames are on the AGENTS.md secret-safety list, and the host was never the point of the evidence, so it is a `<project>` placeholder now. The comment gains the source pointer for the 403 path, and the test pins the 403 sentence so the correction cannot silently regress.
|
All three applied in ec5d984. Each verified against source before changing anything. 403 for signed-in non-members (P2) — correct, and it was the worst of the two failures to leave undocumented. Output inside the bash fence (P2) — correct. The Hostname (P1) — correct. Internal hostnames are on the AGENTS.md secret-safety list and the host was never the load-bearing part of the evidence; the 302 shape was. It is Re-ran the finding's own reproduction against the amended page afterwards, not just the test suite: the verification command extracted verbatim from the page still prints |
kwakayama
left a comment
There was a problem hiding this comment.
Findings
-
[medium] Handle
veryfront.orgsign-in redirects.docs/getting-started/deploy-project.md:68-70states that every protected environment redirects tohttps://veryfront.com/sign-in. That is false for preview hosts on*.preview.veryfront.org:buildProxyAuthRedirectUrlselectshttps://veryfront.org/sign-infor those hosts (src/proxy/proxy-access-control.ts:187-218, covered atsrc/proxy/proxy-access-control.test.ts:102-120). State that the sign-in apex depends on the deployment host, and add a documentation assertion covering the.orgcase. -
[medium] Do not require a
200from the environment root for every successful public deployment.docs/getting-started/deploy-project.md:135presents200as the universal public-environment result for<environment-url>. Deployments without page routes are supported: deployment selects no readiness route when none exists (cli/shared/deployment/deploy-project.ts:1293-1304), and the focused test explicitly treats such projects as having no browser URL (cli/shared/deployment/deploy-project.test.ts:635-650). A successful API-only deployment can therefore have no successful root-page response. Tell readers to probe a route their project serves and validate that route's expected status.
| Category | Score |
|---|---|
| Correctness | 30/40 |
| Tests | 16/20 |
| Reliability/security | 14/15 |
| Maintainability | 14/15 |
| Scope/docs | 8/10 |
| Total | 82/100 |
Review-Gate:
Reviewer: Codex
Reviewed-SHA: ec5d984
Score: 82/100
Actionable-Findings: 2
Verdict: REQUEST_CHANGES
# Conflicts: # tests/docs/guide-content.test.ts
|
Both findings verified against the source before acting. Both citations are accurate; nothing to push back on. Pushed as Finding 1 — sign-in apex depends on the deployment hostCitation confirmed. One nuance I kept the prose honest about rather than inverting the rule: Finding 2 — no universal
|
|
CI follow-up: green, and one more commit.
Flake, not a regression. The first run on this SHA came back with The shard log contains no A dangling promise, no assertion. Two independent reasons it cannot be this branch:
Re-ran the failed jobs only, no code change: Now: 27 pass, 0 fail, 6 skipping. Branch is Still not re-queued — that call is yours. |
The validator this PR adds found three em dashes that reached main while no authoring-time check existed: two from #3606 in project-structure.md and one from #3616 in deploy-project.md. Rewritten as sentences and parentheses with the meaning unchanged. The rebase also took main's 'Verify it worked' section rather than this branch's older copy, so #3616's reviewed route-probing guidance survives.
Dogfood finding (round 2, item 30) against published
0.1.1229.The symptom
The Deploy project page tells the reader to check the preview before deploying,
then to verify the deployment with
curl -sSf <environment-url>. On a defaultVeryfront Cloud project neither step does what the page implies.
Veryfront Cloud creates
preview,staging, andproductionas protected.A protected environment serves only a browser signed in to Veryfront as a
project member. Every other request gets a
302to the sign-in page, on everypath including API routes:
VERYFRONT_API_TOKENdoes not change that — it authenticates the CLI againstthe Cloud API, not deployment traffic.
The page never said any of this, which made its own verification step worse
than useless.
curldoes not treat a302as a failure, so the page'scurl -sSf <environment-url>exits 0 with an empty body whether or not thedeployment works:
A reader following the page sees a green exit code and no output, and concludes
the deploy is fine. Same for the
/api/ag-uiPOST directly below it.The change
at which the page first asks the reader to check the preview. It names the
default, the redirect, why the API token is irrelevant to it, and the Studio
switch (Environments → Public Environment → Make Public) that opens an
environment up.
sign-in redirect is visible rather than passing silently, and says outright
why the bare
curl -sSfform must not be used.Docs and one test only. No source change.
Relationship to #3576
The finding's other half — "
veryfront opendoes not verify the deployment" —was already fixed on
mainby #3576, andopen's behaviour is confirmedunchanged on the published CLI:
That fix has not reached the live site, so it ships to readers together with
this one.
Verification
The regression test failed first on
main's copy of the page (Expected actual: "…" to contain: "protected by default"), then passed after the edit.Proof against the original symptom, not just the test: the new command was
copied verbatim out of the built page and run against a real protected
environment, and it printed exactly what the page now says it will —
Also green locally:
deno test --no-check -A tests/docs/(only unrelatednetwork-sandbox failure in
guide-examples.test.ts),deno fmt --check(4992files),
deno lint,scripts/docs/validate-guides.ts,scripts/lint/check-doc-links.ts(1232 links OK).Live URL to check after merge
This page is synced into
veryfront-docsasdocs/code/getting-started/deploy-project.mdand published at:
https://veryfront.com/docs/code/getting-started/deploy-project
That page must show an Environment access section and a Verify it worked
step whose command contains
%{http_code} %{redirect_url}. Note that the livepage is currently ahead of
veryfront-docs@mainand behindveryfront-code@mainfor this file, so the sync is worth confirming rather than assuming.
Summary by CodeRabbit
Documentation
.comand.orgenvironments.Tests